Skip to content

♻️ Basic cleaning#368

Draft
fbordeu wants to merge 94 commits intoProtoV1from
BasicCleaning
Draft

♻️ Basic cleaning#368
fbordeu wants to merge 94 commits intoProtoV1from
BasicCleaning

Conversation

@fbordeu
Copy link
Copy Markdown
Collaborator

@fbordeu fbordeu commented Apr 8, 2026

Thanks for contributing! Please make sure your PR title and content follow the guidelines.
Leave this checklist below in your PR description and tick the corresponding boxes.

Checklist

  • Typing enforced
  • Documentation updated
  • Changelog updated
  • Tests and Example updates
  • Coverage should be 100%

✅ PR Title Format

Your PR title must start with one of the following emojis to indicate the type of change:

  • 🐛 Bug fix
  • 📄 Documentation
  • 🎉 New feature or initial commit
  • 🚀 Performance or deployment
  • ♻️ Refactor or cleanup
  • 📦 Packaging or dependency management (e.g., pyproject.toml, conda, Poetry, pre-commit)

Example:
🎉 Add MMD-based split method


📋 Description (optional)

Please explain what your PR does:

  • What problem does it solve?
  • Any breaking changes?
  • How was it tested?

🔗 Related issues (optional)

Closes #___

@fbordeu fbordeu requested a review from a team as a code owner April 8, 2026 11:44
@fbordeu fbordeu changed the title Basic cleaning ♻️ Basic cleaning Apr 8, 2026
@bstaber
Copy link
Copy Markdown
Contributor

bstaber commented Apr 8, 2026

🧹

dependabot Bot and others added 2 commits April 14, 2026 14:14
…ss 1 directory (#370)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Fabien Casenave <[email protected]>
@casenave
Copy link
Copy Markdown
Member

@fbordeu can you use draft mode while you still work on the PR ? Here, tests fails and the PR is in Open mode (it's for use to have the signal to start the review :-) )

@fbordeu fbordeu marked this pull request as draft April 14, 2026 21:55
@fbordeu fbordeu force-pushed the BasicCleaning branch 2 times, most recently from 4f972ad to ae1ca7b Compare April 16, 2026 08:21
@bstaber
Copy link
Copy Markdown
Contributor

bstaber commented Apr 17, 2026

image 🚀 🚀 🚀

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 17, 2026

Codecov Report

❌ Patch coverage is 99.46809% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/plaid/containers/sample.py 97.14% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

prediction[count][fn] = op.dot(sample_pred.get_field(fn))
for sn in out_scalars_names:
prediction[count][sn] = sample_pred.get_scalar(sn)
prediction[count][sn] = sample_pred.get_global(sn)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw you changed the benchmarks, but the version of PLAID is fixed for theses benchmarks, and documented for each one. We shoud keep the original synthax

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, you want to keep get_scalar? the old implementation is just a redirection to get_global.

self.features.get_global(name)

and a global can be a numpy array; not a scalar. I think get_global is more appropriate.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what was the exact behavior for each version tagged for each benchmark, so we're better off keeping the original code

@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
3 out of 4 committers have signed the CLA.

✅ casenave
✅ fbordeu
✅ xroynard
❌ dependabot[bot]
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants